Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add timeout support to libhv adapter. #1109

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Add timeout support to libhv adapter. #1109

merged 3 commits into from
Sep 21, 2022

Conversation

michael-grunder
Copy link
Collaborator

Add timeout support to our libhv adapter.

See: #904

@michael-grunder
Copy link
Collaborator Author

michael-grunder commented Sep 8, 2022 •
edited
Loading

cc @ithewei This is my attempt to add timeout support to your adapter. I'm unfamiliar with the library so please let me know if my usage is correct.

Edit: I think this logic isn't ideal if users update the timeouts multiple times (as it will add a bunch of timers). Is there a mechanism in libhv to find an already existing timer?

@ithewei
Copy link

ithewei commented Sep 9, 2022

cc @ithewei This is my attempt to add timeout support to your adapter. I'm unfamiliar with the library so please let me know if my usage is correct.

Edit: I think this logic isn't ideal if users update the timeouts multiple times (as it will add a bunch of timers). Is there a mechanism in libhv to find an already existing timer?

You are right, and you can use htimer_reset to update timeout multiple times.

@michael-grunder
Copy link
Collaborator Author

@ithewei If you don't mind taking a look at the new timeout logic that would be great. In reading the libhv source code I saw that there are several "built-in" timers in the hio_t structure, but it was unclear to me whether I could safely use any of them for this purpose.

@bjosv Let me know if anything jumps out at you. It should now be possible/efficient to update timeouts arbitrarily when using libhv.

@michael-grunder
Add timeout support to libhv adapter.
d1cc536 
See: #904
@michael-grunder
Extended timer support for our libhv adapter.
f202b80 
Keep track of any timer we have set so we can just update it if the user
changes the timeout, rather than continuously adding new timers.

The semantics differ slightly from other adapters as libhv disallows
zero'd timers.
@michael-grunder
Minor refactor of timer logic
6e4f81d
bjosv
bjosv approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Maybe example users would get helped by a comment in debugCallback() that we expect a NULL reply since DEBUG SLEEP should timeout..
..but it LGTM as is.

@michael-grunder michael-grunder merged commit 68b29e1 into master Sep 21, 2022
@michael-grunder michael-grunder deleted the libhv-timer branch September 21, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjosv bjosv bjosv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michael-grunder @ithewei @bjosv